Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the JS benchmark harness to support easily parameterizing concurrency level (for planned concurrency-related driver changes) and adds new “fixed query count / variable concurrency” benchmark variants.
Changes:
- Introduces a centralized JS benchmark entrypoint (
benchmark/logic/benchmark.js) and updates existing benchmark modules to export a common function signature. - Updates benchmarker runner configs and wrapper scripts to call the centralized entrypoint (benchmark names no longer include
.js). - Adds new
*_fixedbenchmark variants in runner configs to sweep concurrency levels while keeping query count fixed.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| benchmark/runner-config/scylladb-driver/config.yml | Switches backend commands to the centralized benchmark entrypoint; adds *_fixed variants. |
| benchmark/runner-config/cassandra-driver/config.yml | Same as above for the cassandra-driver backend; adds *_fixed variants. |
| benchmark/runner-config/run-js-benchmark.sh | Wrapper now calls benchmark/logic/benchmark.js with <driver> <benchmark-name> <N>. |
| benchmark/runner-config/run-js-benchmark-fixed.sh | New wrapper for concurrency-sweep scenarios (step controls concurrency). |
| benchmark/runner-config/config.yml | Adds benchmark definitions for *_fixed variants and adjusts default run count. |
| benchmark/logic/benchmark.js | New centralized dispatcher that loads the driver, creates a client, and invokes the selected benchmark module. |
| benchmark/logic/insert.js | Converted to exported benchmark function with default step count. |
| benchmark/logic/concurrent_insert.js | Converted to exported benchmark function; now accepts concurrencyLevel. |
| benchmark/logic/select.js | Converted to exported benchmark function delegating to parametrized_select. |
| benchmark/logic/parametrized_select.js | Refactored to accept (cassandra, client, rowCount, stepCount) rather than argv. |
| benchmark/logic/large_select.js | Converted to exported benchmark function delegating to parametrized_select. |
| benchmark/logic/paging.js | Converted to exported benchmark function with default step count. |
| benchmark/logic/concurrent_paging.js | Converted to exported benchmark function; now takes runner-provided concurrencyLevel. |
| benchmark/logic/batch.js | Converted to exported benchmark function; clarifies batch-size constant naming/comment. |
| benchmark/logic/deser.js | Converted to exported benchmark function with default step count. |
| benchmark/logic/concurrent_deser.js | Converted to exported benchmark function; now accepts concurrencyLevel. |
| benchmark/logic/ser.js | Converted to exported benchmark function with default step count. |
| benchmark/logic/concurrent_ser.js | Converted to exported benchmark function; now accepts concurrencyLevel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
225088c to
ea74b6c
Compare
|
The default stepCount values vary widely across modules (e.g. batch.js: 3,000,000 vs concurrent_paging.js: 1,280) with no explanation. Since run-js-benchmark-fixed.sh always uses these defaults, it's worth adding a comment in each module clarifying the intended scale. |
Those values were taken from the old python runner, and were determined to be the sweet spot, where the benchmark takes around 1-2 minutes to execute |
ea74b6c to
ae4385c
Compare
ae4385c to
57fc30b
Compare
ffcca89 to
1e687bc
Compare
There was a problem hiding this comment.
♻️ Can we extract the refactor that changes the invocation from using insert.js to insert to a separate commit?
There was a problem hiding this comment.
From what other changes that are present in that commit?
| const iterCnt = parseInt(process.argv[3]); | ||
| // Expectantly determined max batch size, that doesn't cause database error. | ||
| const step = 3971; | ||
| module.exports = function (cassandra, client, stepCount, _concurrencyLevel) { |
There was a problem hiding this comment.
⛏️ Here you use underscore in _concurrencyLevel, and elsewhere not.
There was a problem hiding this comment.
In non concurrent benchmarks I ignore this parameter. For this reason I use underscore to indicate that
benchmark/logic/benchmark.js
Outdated
| const stepCount = process.argv[4] !== "default" ? parseInt(process.argv[4], 10) : undefined; | ||
| const concurrencyLevel =process.argv[5] !== "default" ? parseInt(process.argv[5], 10) : 100; |
There was a problem hiding this comment.
♻️ Let's have separate constants: defaultConcurrencyLevel, defaultStepCount and advise to adjust them as needed. Then, formulas in concurrencyLevel will not have to be ever touched.
There was a problem hiding this comment.
While defaultConcurrencyLevel makes sense, I'm not sure why you want defaultStepCount to be defined as constants.
fca37ed to
593c31b
Compare
This commit moves the entry points for individual benchmarks to separate files. This change will allows us to easily create new benchmarks, that are parametrized on concurrency level, without the need to create a separate copy of the files.
This adds a new version of the existing benchmarks, that try to parameterize the concurrency level, rather then the number of iterations.
This benchmark was still using it's own row check count, instead of the one from utils
c621a25 to
a3a795c
Compare
Refs: #73
With planned changes that touch the concurrency, we would want to see the driver performance across different concurrency levels.
This PR introduces a refactor to the current benchmarks, which would allow us to add benchmarks parameterised on the concurrency level easily, and adds those benchmarks.
I would recommend reviewing the first commit in vscode (or similar), to better see what changes and what's kept unchanged
Introduces changes that were generated with LLM.
Example results [with not yet published PR] (x axis is the concurrency level on the benchmark) - I need to add beter labeling support to the benchmarking tool)